Skip to content

Improved pc_autosave interval#3389

Closed
MrKeiKun wants to merge 1 commit into
HerculesWS:masterfrom
MrKeiKun:fix-166-pc_autosave-interval
Closed

Improved pc_autosave interval#3389
MrKeiKun wants to merge 1 commit into
HerculesWS:masterfrom
MrKeiKun:fix-166-pc_autosave-interval

Conversation

@MrKeiKun

@MrKeiKun MrKeiKun commented Sep 1, 2025

Copy link
Copy Markdown
Contributor

Pull Request Prelude

Changes Proposed

  1. Added set_interval function to timer interface:
  2. Improved pc_autosave logic:

Issues addressed: #166

Signed-off-by: Lorenzo Buitizon <the.keikun@gmail.com>
@MrKeiKun MrKeiKun force-pushed the fix-166-pc_autosave-interval branch from 33a31bf to dee8b95 Compare April 28, 2026 10:44
@skyleo

skyleo commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

I don't really understand the point of this, can you try explaining the advantage?

Also doesn't this destroy the heap property that timers are in? (at least I think they are in a heap)

@MrKeiKun

MrKeiKun commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

I don't really understand the point of this, can you try explaining the advantage?

The original code calls timer->add on every execution, unconditionally allocating a new timer slot even when the computed interval is identical (player count hasn't changed). With the persistent interval timer, the common case (no player count change) becomes a no-op. timer->set_interval is only called when the interval actually differs.

Also doesn't this destroy the heap property that timers are in? (at least I think they are in a heap)

No, it doesn't disturb the heap. The heap (timer_heap) is a min-heap ordered by tick (next fire time), see the DIFFTICK_MINTOPCMP comparator in timer.c. timer_set_interval only modifies timer_data[tid].interval, which is never a heap key. The interval field is read only inside do_timer to compute the next tick after a fire event. Changing it mid-life has no effect on the current heap ordering.

but overall, you're correct to question it. there's no meaningful advantage, and the complexity cost outweighs the gain. We should close this.

@MrKeiKun MrKeiKun closed this Jun 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants